Fix MCP stdio protocol violation during startup (Docker/langchain compatibility) #285
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds buffering and initialization guards to the MCP transport so logs, progress, and custom notifications are queued until MCP initialization; transport creation is moved earlier and exported globally; feature-flag background timer and async logs are silenced to avoid emitting during initialization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/index.ts (1)
14-17: Consider consolidating the dual buffering mechanism.The code currently maintains two separate buffers:
deferredMessagesarray in index.ts (lines 14-17)messageBufferinside FilteredStdioServerTransportBoth are flushed in the oninitialized callback (lines 113-117). While this works correctly, you could simplify by using only the transport's internal buffering mechanism and calling
transport.sendLog()directly instead ofdeferLog().Example simplification:
-const deferredMessages: Array<{level: string, message: string}> = []; -function deferLog(level: string, message: string) { - deferredMessages.push({level, message}); -} - async function runServer() { try { ... try { - deferLog('info', 'Loading configuration...'); + transport.sendLog('info', 'Loading configuration...'); await configManager.loadConfig(); - deferLog('info', 'Configuration loaded successfully'); + transport.sendLog('info', 'Configuration loaded successfully'); ... } ... server.oninitialized = () => { transport.enableNotifications(); - - // Flush all deferred messages from both index.ts and server.ts - while (deferredMessages.length > 0) { - const msg = deferredMessages.shift()!; - transport.sendLog('info', msg.message); - } flushDeferredMessages(); ... };Also applies to: 50-63, 113-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/custom-stdio.ts(3 hunks)src/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-25T16:02:52.184Z
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler. This is the official recommended approach according to the documentation for ensuring complete isolation between requests and preventing request ID collisions in concurrent scenarios.
Applied to files:
src/index.ts
📚 Learning: 2025-05-25T16:02:52.184Z
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the pattern of calling both transport.close() and server.close() in the request close event handler is the official recommended approach according to the documentation at https://github.com/modelcontextprotocol/typescript-sdk.
Applied to files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (1)
src/custom-stdio.ts (1)
FilteredStdioServerTransport(18-410)
🔇 Additional comments (4)
src/custom-stdio.ts (3)
280-297: LGTM! Buffering implementation correctly prevents protocol violation.The buffering logic properly defers sendLog calls until initialization completes, ensuring MCP protocol compliance. The timestamp-based ordering guarantees chronological replay.
330-333: LGTM! Correctly drops transient progress notifications.Progress notifications are appropriately dropped rather than buffered, as replaying stale progress values after initialization would be misleading.
366-369: LGTM! Correctly drops transient custom notifications.Custom notifications are appropriately dropped rather than buffered, preventing protocol violations while avoiding the replay of potentially stale notification data.
src/index.ts (1)
42-47: Early transport creation correctly fixes protocol violation.The transport creation before config loading ensures buffering is available from the start, correctly addressing the MCP protocol violation. Type safety is already properly handled with a global type declaration in
src/types.ts.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| */ | ||
| public sendProgress(token: string, value: number, total?: number) { | ||
| // Don't send progress before initialization - would break MCP protocol | ||
| if (!this.isInitialized) { |
There was a problem hiding this comment.
Suggestion: The progress notification method currently ignores the disableNotifications flag, so clients explicitly configured to have notifications disabled (e.g., Cline/vscode/claude-dev) will still receive progress JSON-RPC messages, which contradicts the intent of configureForClient and can break those clients; you should also short‑circuit when notifications are disabled. [logic error]
Severity Level: Minor
| if (!this.isInitialized) { | |
| // Also respect clients that have notifications disabled entirely | |
| if (!this.isInitialized || this.disableNotifications) { |
Why it matters? ⭐
This is a real behavioral bug, not cosmetic. The class has an explicit
disableNotifications flag set in configureForClient for Cline/vscode/claude-dev,
and both sendLog and sendLogNotification already short-circuit on that flag,
meaning "notifications disabled" is intended to apply globally. However,
sendProgress only checks isInitialized and still emits JSON-RPC progress
notifications even when disableNotifications is true, which contradicts the
stated behavior and can still break those same clients. Adding the
this.disableNotifications check aligns sendProgress with the rest of the
class' behavior and prevents unwanted notifications from being sent.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/custom-stdio.ts
**Line:** 331:331
**Comment:**
*Logic Error: The progress notification method currently ignores the `disableNotifications` flag, so clients explicitly configured to have notifications disabled (e.g., Cline/vscode/claude-dev) will still receive progress JSON-RPC messages, which contradicts the intent of `configureForClient` and can break those clients; you should also short‑circuit when notifications are disabled.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| */ | ||
| public sendCustomNotification(method: string, params: any) { | ||
| // Don't send custom notifications before initialization - would break MCP protocol | ||
| if (!this.isInitialized) { |
There was a problem hiding this comment.
Suggestion: The custom notification method does not check the disableNotifications flag, so even when a client has been configured to disable all notifications, custom JSON-RPC notifications will still be sent, leading to inconsistent behavior and potential protocol issues for those clients; this should also bail out when notifications are disabled. [logic error]
Severity Level: Minor
| if (!this.isInitialized) { | |
| // Also respect clients that have notifications disabled entirely | |
| if (!this.isInitialized || this.disableNotifications) { |
Why it matters? ⭐
Same story here: configureForClient explicitly sets disableNotifications
for certain clients and logs "Notifications disabled", and both sendLog and
sendLogNotification honor that flag. sendCustomNotification only checks
isInitialized and will still push arbitrary JSON-RPC notifications over
stdout even when disableNotifications is true, which is inconsistent and
undermines the purpose of the flag. Adding a this.disableNotifications
short-circuit fixes a real logic inconsistency and prevents custom notifications
from leaking to clients that are supposed to receive none.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/custom-stdio.ts
**Line:** 367:367
**Comment:**
*Logic Error: The custom notification method does not check the `disableNotifications` flag, so even when a client has been configured to disable all notifications, custom JSON-RPC notifications will still be sent, leading to inconsistent behavior and potential protocol issues for those clients; this should also bail out when notifications are disabled.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
The MCP protocol requires the client to send the first message. Writing to stdout before the client's initialization request breaks the protocol handshake, causing 'BrokenResourceError' in clients like langchain-mcp-adapters. Root cause: Transport was created AFTER config/feature flags initialization, so logger.* calls during startup wrote directly to stdout (bypassing buffering) or called sendLog() which also wrote to stdout without checking isInitialized. Additional issue: Async feature flag fetch operations would log messages AFTER the client had already started closing, causing timing conflicts. Fixes: 1. Move FilteredStdioServerTransport creation before config loading 2. Buffer sendLog() messages when isInitialized is false 3. Guard sendProgress/sendCustomNotification when not initialized 4. Add .unref() to feature flag refresh interval for clean process exit 5. Remove async logging from feature flag fetch/save operations Tested with langchain-mcp-adapters - now passes where it previously failed. Fixes #issue-reported-by-ever1022
wonderwhy-er
force-pushed
the
fix/feature-flags-stdio-issue
branch
from
4ee1411 to
6c5a467
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
125-148: Remove redundant direct stdout write in error handler; logger already handles it.The error handler at lines 125-148 calls
logger.error()at line 135, which properly routes throughglobal.mcpTransport.sendLog()when available. The subsequent directprocess.stdout.write()at line 142 is redundant—it duplicates the logging already performed and violates the protocol by bypassing the sendLog() buffering mechanism. Sinceglobal.mcpTransportis initialized at line 47 beforeserver.connect()executes, the logger will correctly handle the error notification. Remove lines 141-147 and let the existinglogger.error()call handle all error notification.
♻️ Duplicate comments (2)
src/custom-stdio.ts (2)
330-333:disableNotificationsflag is not checked here.As noted in a previous review, this guard should also respect the
disableNotificationsflag for consistency withsendLogandsendLogNotification.- if (!this.isInitialized) { + if (!this.isInitialized || this.disableNotifications) {
366-369:disableNotificationsflag is not checked here.Same issue as
sendProgress— should also checkdisableNotificationsfor consistency with other notification methods.- if (!this.isInitialized) { + if (!this.isInitialized || this.disableNotifications) {
🧹 Nitpick comments (1)
src/index.ts (1)
49-64: **Inconsistent indentation in config loading block.**The lines 49-64 have extra indentation (6 spaces instead of expected 4 spaces) compared to the surrounding code. This appears to be a formatting inconsistency that should be aligned with the rest of the function.- try { - deferLog('info', 'Loading configuration...'); - await configManager.loadConfig(); - deferLog('info', 'Configuration loaded successfully'); - - // Initialize feature flags (non-blocking) - deferLog('info', 'Initializing feature flags...'); - await featureFlagManager.initialize(); - } catch (configError) { - deferLog('error', `Failed to load configuration: ${configError instanceof Error ? configError.message : String(configError)}`); - if (configError instanceof Error && configError.stack) { - deferLog('debug', `Stack trace: ${configError.stack}`); - } - deferLog('warning', 'Continuing with in-memory configuration only'); - // Continue anyway - we'll use an in-memory config - } + try { + deferLog('info', 'Loading configuration...'); + await configManager.loadConfig(); + deferLog('info', 'Configuration loaded successfully'); + + // Initialize feature flags (non-blocking) + deferLog('info', 'Initializing feature flags...'); + await featureFlagManager.initialize(); + } catch (configError) { + deferLog('error', `Failed to load configuration: ${configError instanceof Error ? configError.message : String(configError)}`); + if (configError instanceof Error && configError.stack) { + deferLog('debug', `Stack trace: ${configError.stack}`); + } + deferLog('warning', 'Continuing with in-memory configuration only'); + // Continue anyway - we'll use an in-memory config + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/custom-stdio.ts(3 hunks)src/index.ts(1 hunks)src/utils/feature-flags.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-25T16:02:52.184Z
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler. This is the official recommended approach according to the documentation for ensuring complete isolation between requests and preventing request ID collisions in concurrent scenarios.
Applied to files:
src/index.ts
📚 Learning: 2025-05-25T16:02:52.184Z
Learnt from: tillahoffmann
Repo: wonderwhy-er/DesktopCommanderMCP PR: 137
File: src/index.ts:160-162
Timestamp: 2025-05-25T16:02:52.184Z
Learning: In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the pattern of calling both transport.close() and server.close() in the request close event handler is the official recommended approach according to the documentation at https://github.com/modelcontextprotocol/typescript-sdk.
Applied to files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (1)
src/custom-stdio.ts (1)
FilteredStdioServerTransport(18-410)
🔇 Additional comments (5)
src/utils/feature-flags.ts (2)
49-51: Good use ofunref()to prevent blocking process exit.This ensures the periodic refresh timer doesn't keep the process alive after the MCP client disconnects. The explicit comment explaining why is helpful for future maintainers.
114-114: Silent logging approach is appropriate for async operations.Removing log statements from async code paths (fetch and cache save) that may execute after MCP client disconnection is the right call. These operations now fail silently, relying on the debug-level error logging in the catch blocks instead.
Also applies to: 139-141, 160-160
src/custom-stdio.ts (1)
288-297: Buffering logic correctly mirrors the console redirection pattern.The implementation properly buffers messages with level, structured args, and timestamp, matching the existing
setupConsoleRedirectionpattern. This ensures allsendLogcalls during startup are captured and replayed after MCP initialization.src/index.ts (2)
42-47: Correct fix: transport creation moved before config/feature-flag initialization.This ensures
global.mcpTransportis set before any code path can triggerlogger.*calls, allowing theFilteredStdioServerTransportto buffer all messages properly. The explicit comment documents the ordering constraint for future maintainers.
107-122: Well-structured event-driven initialization completion.The use of
server.oninitializedcallback correctly defers notification enabling and message flushing until the MCP handshake is complete. This follows the learnings about proper MCP protocol handling with the TypeScript SDK.
User description
Problem
After commit cc89540 (v0.2.19+), Desktop Commander fails to work with certain MCP clients like
langchain-mcp-adaptersand in Docker environments. The client receivesanyio.BrokenResourceErrorwhen trying to fetch tools.Reported by @ever1022 in GitHub issue comments.
Root Cause
The MCP stdio protocol requires the client to send the first message. Any server output to stdout before the client's initialization request breaks the protocol handshake.
The issue was a timing problem in the initialization sequence:
When
featureFlagManager.initialize()ran,global.mcpTransportdidn't exist yet, sologger.tsfell back to writing JSON-RPC notifications directly to stdout. Additionally, even when the transport existed,sendLog()didn't checkisInitializedbefore writing.Solution
Move transport creation before config loading - so all logging gets properly buffered from the start
Buffer
sendLog()messages - whenisInitializedis false, store in buffer for replay after MCP handshakeGuard
sendProgress/sendCustomNotification- silently drop when not initializedTesting
Created a test script that:
Before fix: Server wrote JSON-RPC notifications to stdout before client message
After fix: No premature stdout output, server responds correctly
Files Changed
src/index.ts- Move transport creation before config/feature flags initsrc/custom-stdio.ts- Add buffering tosendLog(), guardsendProgress/sendCustomNotificationCodeAnt-AI Description
Prevent MCP server from writing to stdout before client handshake
What Changed
Impact
✅ Fewer MCP startup failures✅ Reliable connections with langchain MCP adapters and Docker clients✅ No stray server logs before MCP client handshake💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.